Log tool call name on success, not just failure#1256
Merged
Conversation
Add a LoggerMessage-based ToolCallCompleted log at Information level that captures the tool name on successful tool call completion, matching the existing ToolCallError pattern for failures. Also add the log call to the task-augmented tool execution path. Add test Logs_Tool_Name_On_Successful_Call to verify the new log message. Co-authored-by: halter73 <54385+halter73@users.noreply.github.com>
Address code review: only log ToolCallCompleted when the task-augmented tool execution result indicates success (not IsError). Co-authored-by: halter73 <54385+halter73@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Capture tool call name in ILogger logs on success
Log tool call name on success, not just failure
Feb 10, 2026
stephentoub
reviewed
Feb 10, 2026
stephentoub
reviewed
Feb 10, 2026
tests/ModelContextProtocol.Tests/Configuration/McpServerBuilderExtensionsToolsTests.cs
Show resolved
Hide resolved
stephentoub
previously approved these changes
Feb 10, 2026
halter73
reviewed
Feb 10, 2026
Contributor
halter73
left a comment
There was a problem hiding this comment.
Please include CallToolResult.IsError in the log.
Address reviewer feedback: - Include CallToolResult.IsError as a parameter in the ToolCallCompleted log message (@halter73) - Always log ToolCallCompleted when InvokeAsync doesn't throw, including when IsError=true (@halter73) - Skip ToolCallCompleted in outer handler for task-augmented calls to avoid double-logging; the async ExecuteToolAsTaskAsync logs separately - Add ReturnIsError tool for testing IsError=true without throwing - Add Logs_Tool_Name_With_IsError_When_Tool_Returns_Error test - Add TaskPath_Logs_Tool_Name_On_Successful_Call test - Add TaskPath_Logs_Tool_Name_With_IsError_When_Tool_Returns_Error test - Add TaskPath_Logs_Error_When_Tool_Throws test - Update tool count assertions for new ReturnIsError tool Co-authored-by: halter73 <54385+halter73@users.noreply.github.com>
Contributor
Author
Done in c18f406. The log message is now |
halter73
reviewed
Feb 10, 2026
tests/ModelContextProtocol.Tests/Server/ToolTaskSupportTests.cs
Outdated
Show resolved
Hide resolved
Contributor
|
@copilot Please address my comment about using |
Use GetTaskResultAsync to wait for task completion instead of arbitrary Task.Delay(500) calls, making tests deterministic and faster. Co-authored-by: halter73 <54385+halter73@users.noreply.github.com>
Add GetPromptCompleted/GetPromptError and ReadResourceCompleted/ ReadResourceError LoggerMessage methods, symmetric with the existing tool logging. Wrap prompt and resource filter pipelines with try/catch to log when handlers succeed or throw unhandled exceptions. Add tests: - Logs_Prompt_Name_On_Successful_Call - Logs_Prompt_Name_When_Prompt_Throws - Logs_Resource_Uri_On_Successful_Read - Logs_Resource_Uri_When_Resource_Throws Co-authored-by: halter73 <54385+halter73@users.noreply.github.com>
Contributor
Author
Both addressed: |
halter73
approved these changes
Feb 10, 2026
stephentoub
previously approved these changes
Feb 10, 2026
halter73
requested changes
Feb 11, 2026
…n types Remove the exception type filter (when clause) from the error logging catch blocks for prompts and resources, and add a second catch block for tools, so that OperationCanceledException and McpProtocolException are also logged with tool/prompt/resource names. Behavior is unchanged: tools still produce IsError results for non-cancel/non-protocol exceptions, and prompts/resources still rethrow all exceptions. Add tests verifying error logging and LogRequestHandlerException for OperationCanceledException and McpProtocolException in all three handler types. Co-authored-by: halter73 <54385+halter73@users.noreply.github.com>
…dException When a tool throws OperationCanceledException but the passed-in cancellation token is not canceled, produce an errored CallToolResult (IsError=true) instead of rethrowing. This prevents non-user-initiated cancellations from propagating as protocol errors. User-initiated cancellations (where cancellationToken.IsCancellationRequested is true) still rethrow as before. Co-authored-by: halter73 <54385+halter73@users.noreply.github.com>
halter73
reviewed
Feb 11, 2026
Consolidate the OperationCanceledException, general exception, and fallback catch blocks into a single catch (Exception e) with: - One call to ToolCallError - Conditional rethrow for user-initiated cancellation or McpProtocolException - One return of errored CallToolResult for everything else Co-authored-by: halter73 <54385+halter73@users.noreply.github.com>
halter73
approved these changes
Feb 11, 2026
stephentoub
approved these changes
Feb 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ToolCallCompletedlog message inMcpServerImpl.csthat captures the tool name andIsErroron tool call completionCallToolResult.IsErroras a parameter in theToolCallCompletedlogToolCallCompletedwhen InvokeAsync doesn't throw, even withIsError=trueToolCallCompletedin outer handler for task-augmented calls to avoid double-loggingGetTaskResultAsyncinstead ofTask.DelayGetPromptCompleted/GetPromptError) with testsReadResourceCompleted/ReadResourceError) with testsToolCallError,GetPromptError,ReadResourceErrorto log for ALL exception typesOperationCanceledExceptionandMcpProtocolExceptionverifying both named error log andLogRequestHandlerExceptionCallToolResultforOperationCanceledExceptionwhen passed-in token is NOT canceled, with regression testcatch (Exception e)with conditional rethrowOriginal prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.